Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OoT] OoT Object changes & small changes/fixes/removals #95

Merged
merged 60 commits into from
Nov 17, 2022

Conversation

Yanis42
Copy link
Contributor

@Yanis42 Yanis42 commented May 19, 2022

List of changes:

  • Added ObjectList.xml which contains every objects from OoT, you have the name, the decomp ID and something called ObjectKey, it's used to identify an object, this should never change (the format is obj_ + the debug name of the object)
  • Added ActorList.xml, currently unused outside identifying an actor, it will be used whenever PR 56 is merged though, the format of this file is more complex because there's every parameters etc, but it's a bit like the object list file
  • Export defines in scenes for the lengths of actor and object lists, makes manual editing easier to do
  • Fixed the scene being automatically selected in blender when you export (at least I think it's fixed)
  • Replaced remaining \t in OoT files with the indent variable introduced with PR 79 (decomp uses 4 spaces)
  • Deleted oot_parse.py and sceneDirectoryParser.py as it's unused
  • Automatically add missing objects to the Room when you export the scene, (hence the ActorList.xml file being here), it's also adding it in the UI
  • Finally, I got kinda got rid of debug names for the object list, now we're generating the list from the XML data, the key is the data saved in the blend (hence why you can't change that, it's there to be future proof for decomp changed) and the names are displayed in the list, makes choosing an object a lot better (for instance, you can type "zelda" and have every objects related to Zelda, see the preview image to see this example in action)
  • Alternate room headers should work with these new features (child/adult day/night + cutscene headers)

TL;DR: new features regarding OoT objects and some fixes/removed unused stuff/enhanced the C output a bit

Object list preview
Actor/Object Lists defines preview

fast64_internal/oot/oot_constants.py Outdated Show resolved Hide resolved
fast64_internal/oot/c_writer/oot_level_c.py Outdated Show resolved Hide resolved
fast64_internal/oot/c_writer/oot_level_c.py Outdated Show resolved Hide resolved
fast64_internal/oot/ObjectList.xml Outdated Show resolved Hide resolved
fast64_internal/oot/oot_utility.py Outdated Show resolved Hide resolved
fast64_internal/oot/oot_utility.py Outdated Show resolved Hide resolved
@Dragorn421 Dragorn421 added the oot Has to do with the Ocarina of Time 64 side label Sep 26, 2022
fast64_internal/oot/data/oot_getters.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_actor_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_actor_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/c_writer/oot_level_c.py Outdated Show resolved Hide resolved
fast64_internal/oot/c_writer/oot_level_c.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_actor_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_actor_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_getters.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_object_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_object_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_getters.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_getters.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_actor_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_object_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/oot_level_writer.py Outdated Show resolved Hide resolved
fast64_internal/oot/oot_object.py Outdated Show resolved Hide resolved
fast64_internal/oot/oot_object.py Outdated Show resolved Hide resolved
fast64_internal/oot/oot_object.py Outdated Show resolved Hide resolved
fast64_internal/oot/oot_operators.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_getters.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_getters.py Outdated Show resolved Hide resolved
fast64_internal/oot/oot_upgrade.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_actor_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_object_data.py Outdated Show resolved Hide resolved
Comment on lines 49 to 51
# validate the legacy list, if there's any None element then something's wrong
if None in self.ootEnumObjectIDLegacy:
raise PluginError("ERROR: Legacy Object List doesn't match!")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note this prevents from removing things from the xml but whatever I guess, can be addressed later if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check makes sense since we want the previous and hardcoded object list, so yea I think it should stay

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we only want it in the case the user has old blends, but it'd prevent fast64 from enabling even if they were starting from scratch and just deleted an object in the xml that they also removed in their repo

again this can be addressed later if needed though, just something to be aware of

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we had ifdefs in python this wouldn't be an issue 😔

fast64_internal/oot/oot_object.py Outdated Show resolved Hide resolved
fast64_internal/oot/oot_level_writer.py Outdated Show resolved Hide resolved
@Yanis42 Yanis42 mentioned this pull request Oct 1, 2022
@Yanis42
Copy link
Contributor Author

Yanis42 commented Oct 2, 2022

found and fixed a minor issue in ootConvertScene(), for obj in sceneObj.children doesn't look for objects parented to a scene but also parented to something else, fixed by using children_recursive (for obj in sceneObj.children_recursive)

fast64_internal/oot/data/oot_actor_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_actor_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_actor_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/data/oot_object_data.py Outdated Show resolved Hide resolved
fast64_internal/oot/oot_level_writer.py Outdated Show resolved Hide resolved
@Yanis42
Copy link
Contributor Author

Yanis42 commented Nov 12, 2022

merged main, is there anything holding this PR from being merged?

Copy link
Contributor

@kurethedead kurethedead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested by exporting a kakariko import and it seems to work fine. I'm not sure exactly where the object list updating is occurring, but I think something needs to communicate to the user how many objects are actually being exported, so they don't get confused when something breaks unexpectedly.

fast64_internal/oot/oot_actor.py Show resolved Hide resolved
fast64_internal/oot/oot_level_writer.py Show resolved Hide resolved
fast64_internal/oot/oot_level_writer.py Show resolved Hide resolved
Copy link
Contributor

@sauraen sauraen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Please merge ASAP.

Tested and it does what it says. I was concerned about support for non-decomp custom actors/objects (which only have a number), but it works and exports them.

One annoyance--when making actors (by adding an empty and changing its type to Actor), or when making other types of objects by this method (even though for scenes and rooms there's an operator available to make them), it says the data is out of date and gives the upgrade button. It works after clicking the button, but things shouldn't be created out of date. Changing the dropdown for the empty type should also do the initialization.

@Dragorn421
Copy link
Contributor

fwiw I don't remember having issues with this PR last time I was actively exchanging about it with Yanis, besides a bit of jank around data upgrading which is tied to the issue mentioned by Sauraen about empty type switching.

So once you guys are happy I +1 merging

@Yanis42
Copy link
Contributor Author

Yanis42 commented Nov 13, 2022

I can add an "Add Actor" button in the tools panel but if the user adds an actor manually this issue will stay and idk how to fix that properly

@kurethedead
Copy link
Contributor

I've made a comment in discord about a way to fix sauraen's problem with object versioning. Otherwise, I still think there should be something that ensures that the user is aware of when objects are auto-added to the object list (error-checking, warning UI?) especially if those added objects would cause a crash.

@Yanis42
Copy link
Contributor Author

Yanis42 commented Nov 15, 2022

I think I understand how this will fix the issue but I'm not sure how to implement that 🤔

Copy link
Contributor

@kurethedead kurethedead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the PR uses the object properties version to handle updates to the OOT object list enum, but its broken currently (#179). The versioning system I suggested in discord also doesn't work, as I realized that old properties that haven't had their default changed are also not accessible in the way I suggested. As a workaround, we can apply the same concept to the objectID property on the OOTObjectProperty itself. Remove the objectID definition, then in code the presence of objProp["objectID"] indicates an un-upgraded property. The property can be updated, then del objProp["objectID"] is called. The exporter can handle both upgraded and un-upgraded versions using this check, so that even if the user doesn't update the export will still be correct.

fast64_internal/oot/oot_scene_room.py Outdated Show resolved Hide resolved
fast64_internal/oot/oot_scene_room.py Outdated Show resolved Hide resolved
fast64_internal/oot/oot_scene_room.py Show resolved Hide resolved
fast64_internal/oot/oot_level_writer.py Outdated Show resolved Hide resolved
fast64_internal/oot/oot_level.py Outdated Show resolved Hide resolved
@Yanis42
Copy link
Contributor Author

Yanis42 commented Nov 16, 2022

I understand how your idea is working but I don't know how to implement that

@Yanis42
Copy link
Contributor Author

Yanis42 commented Nov 16, 2022

Updated the upgrade system with Kure's suggestions, also I thought it'd be fine if I removed the version system completely since the property trick works better, now we just need to figure out a standard way to do this but it can be done later

Changed the UI to make it draw, if it's still using old data everything on the object list will be disabled but visible with the upgrade button at the top, also exporting should work properly if the blend isn't updated yet

Copy link
Contributor

@kurethedead kurethedead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Will merge soon if unless anyone objects.

@kurethedead kurethedead merged commit 925df1a into Fast-64:main Nov 17, 2022
@Yanis42 Yanis42 deleted the objects_and_fixes branch December 5, 2022 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
oot Has to do with the Ocarina of Time 64 side
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants